Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(video): add support for video ad volume control #613

Merged
merged 4 commits into from
Jul 23, 2024

Conversation

adrienlfr
Copy link
Contributor

@adrienlfr adrienlfr commented Jul 22, 2024

Description

I need to be able to control the volume of ads in my application.

Here's the documentation for volume control: Video ad volume control

Related issues

Fixes #580

Release Summary

Implement Video ad volume control

Checklist

  • I read the Contributor Guide
    and followed the process outlined there for submitting PRs.
    • Yes
  • My change supports the following platforms;
    • Android
    • iOS
  • My change includes tests;
    • e2e tests added or updated in __tests__e2e__
    • jest tests added or updated in __tests__
  • I have updated TypeScript types that are affected by my change.
  • This is a breaking change;
    • Yes
    • No

Test Plan

image

Think react-native-google-mobile-ads is great? Please consider supporting the project with any of the below:

  • 👉 Star this repo on GitHub ⭐️
  • 👉 Follow Invertase on Twitter

Copy link

codecov bot commented Jul 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 43.26%. Comparing base (a34c7ba) to head (3ee08f0).
Report is 44 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #613      +/-   ##
==========================================
- Coverage   43.72%   43.26%   -0.45%     
==========================================
  Files          30       31       +1     
  Lines         549      571      +22     
  Branches      151      155       +4     
==========================================
+ Hits          240      247       +7     
- Misses        309      324      +15     

it('throws if setAppVolume is greater then 1', function () {
expect(() => {
admob().setAppVolume(2);
}).toThrowError('The app volume must be a value between 0 and 1 inclusice.');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo inclusice

src/MobileAds.ts Outdated

setAppVolume(volume: number) {
if (volume < 0 || volume > 1)
throw new Error('The app volume must be a value between 0 and 1 inclusice.');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo inclusice

@dylancom
Copy link
Collaborator

Nice addition!

expect(RNGoogleMobileAdsModule.setAppVolume).toBeCalledTimes(1);
});

it('throws if setAppVolume is greater then 1', function () {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

greater than 1

@CLAassistant
Copy link

CLAassistant commented Jul 22, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ adrienlfr
❌ AdrienLefaure
You have signed the CLA already but the status is still pending? Let us recheck it.

@@ -484,3 +484,38 @@ function App() {
```

The `sizes` prop takes an array of [`BannerAdSize`](/reference/admob/banneradsize) types.

### Video ad volume control
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should place this under 'Advanced Usage'

@adrienlfr
Copy link
Contributor Author

Thanks for the review. I've corrected the spelling mistakes and updated the doc.

@dylancom
Copy link
Collaborator

I meant under the "Section" Advanced Usage.
See docs.json:

"Advanced Usage",
      [
        ["Displaying Ads via Hook", "/displaying-ads-hook"],
        ["European User Consent", "/european-user-consent"],
        ["Ad Inspector", "/ad-inspector"],
        ["Impression-level ad revenue", "/impression-level-ad-revenue"]
      ]

@adrienlfr
Copy link
Contributor Author

Sorry, I didn't have the latest version of the branch. So I didn't quite understand what you were asking me. 😄

@dylancom
Copy link
Collaborator

Awesome, thanks for making this library better :)

@dylancom dylancom merged commit c1d821d into invertase:main Jul 23, 2024
9 of 10 checks passed
@mikehardy
Copy link
Collaborator

🎉 This PR is included in version 14.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[🐛] MobileAds.setAppVolume and MobileAds.setAppMuted
4 participants